-
Notifications
You must be signed in to change notification settings - Fork 350
fix: ノートをコピーして歌詞入力などのペーストしたときにノートオブジェクトがペーストされないようにする #2615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: ノートをコピーして歌詞入力などのペーストしたときにノートオブジェクトがペーストされないようにする #2615
Conversation
|
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ: |
|
PRありがとうございます!! もしよければ @terapotan さんもコメントいただけると!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where copying notes from a song resulted in pasting the entire note JSON when pasted outside of the sequencer, by separating out the data formats for sequencer paste (note JSON) and general paste (lyrics text only). Key changes include:
- Serializing notes as JSON and combining lyric text for the clipboard.
- Using a custom MIME type "web application/vnd.voicevox.song-notes" for sequencer paste while providing a plain text fallback.
- Implementing a fallback mechanism for browsers that do not support custom MIME types.
Comments suppressed due to low confidence (1)
src/store/singing.ts:3289
- Consider adding a feature detection or fallback for navigator.clipboard.read to improve compatibility with browsers that do not support this asynchronous Clipboard API method.
const clipboardItems = await navigator.clipboard.read();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ははーーーーーー なるほど、webのクリップボードはこういう感じになってるんですねぇ!!!!
たまに変な挙動するな〜と感じることがあるのですが、こりゃしょうがないわって気持ちになりました!!! ややこしすぎる!!!
主にコード整理周りでコメントしました!
src/store/singing.ts
Outdated
| .filter((note: Note) => noteIds.has(note.id)) | ||
| .map((note: Note) => { | ||
| // idのみコピーしない | ||
| const { id, ...noteWithoutId } = note; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(この行に関係ないコメントです、議論を分けたかったので)
ちょっと提案なのですが、クリップボード周りの処理を関数に切り出してファイルをまとめるのはどうでしょうか 👀
コンテキストが特殊なのと、定数などがいっぱいあるので少なくとも外に出したほうがメンテしやすそうだな〜と。
storeは関数抜き出しするの面倒だったのですが、同名のディレクトリ以下に配置すれば綺麗に移行できることに気付きました!
やることは単純で、
- 新しく
store/singing/ディレクトリを作って、 store/singing.tsをstore/singing/index.tsに移動して- 切り出した関数を
store/singing/clipboard.tsなどに配置する
感じです!実例だとこんな感じ!
切り出した関数は第1引数でcontext: ActionContextを受け取って、こんな感じで渡すとか・・・!
(もちろんstateとかactionsとかだけ渡すのもOKだと思います)
説明テキストとか、MIME Type定数とか、移してまとめられると思うのでキレイになるんじゃないかな〜みたいな!
型とかだいぶパズルになっててややこしいと思うのでなんでも聞いて下さい!
なんなら残しといていただければこっちでちゃちゃっとやっちゃいます 🙏
もしよかったら!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらclipboardHelper.tsに分離する形で修正しました!
リファクタリングもちょっとしたつもりですが、わかりづらければおしらせください…!
https://github.com/romot-co/voicevox/blob/fix/copy_paste_with_custom_mimetype-2591/src/store/singing/clipboardHelper.ts
src/store/singing.ts
Outdated
| // ノートコピー&ペースト用のカスタムMIMEタイプ | ||
| const customMimeType = "web application/vnd.voicevox.song-notes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これはペースト時も共通だから外に出したいかも
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらclipboardHelper側に定数としておく形にいたしました!
src/store/singing.ts
Outdated
| try { | ||
| await navigator.clipboard.writeText(serializedNotes); | ||
| logger.info("Fallback to plain text.", e); | ||
| logger.info("Copied to clipboard as plain text", serializedNotes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ最近気づいたのですが、エラーになる処理の前にロガー書かないと表示されないんですよね 😇
なのでこうかも?
| try { | |
| await navigator.clipboard.writeText(serializedNotes); | |
| logger.info("Fallback to plain text.", e); | |
| logger.info("Copied to clipboard as plain text", serializedNotes); | |
| try { | |
| logger.info("Fallback to plain text.", e); | |
| await navigator.clipboard.writeText(serializedNotes); | |
| logger.info("Copied to clipboard as plain text", serializedNotes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loggerをclipboardHelper側に置くのは微妙に思えたのと、
loggerなくても良いように思えたため、いったんlogger削除いたしました。
問題あれば、copyNotesToClipboardとreadNotesFromClipboardあたりをtry...catchして
スローされたエラーをloggerで表示がいいかなあと…
src/store/singing.ts
Outdated
| for (const item of clipboardItems) { | ||
| if (item.types.includes(customMimeType)) { | ||
| const blob = await item.getType(customMimeType); | ||
| clipboardText = await blob.text(); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // カスタムMIMEタイプが見つからない場合は通常のテキストを試す | ||
| if (!clipboardText) { | ||
| for (const item of clipboardItems) { | ||
| if (item.types.includes("text/plain")) { | ||
| const blob = await item.getType("text/plain"); | ||
| clipboardText = await blob.text(); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ただのコメントです)
処理がにてるから関数切り出しできそう!!
|
@romot-co |
src/store/singing.ts
Outdated
| // カスタムMIMEタイプのデータを読み込む | ||
| const customMimeType = "web application/vnd.voicevox.song-notes"; | ||
| // クリップボードアイテムからカスタムMIMEタイプのデータを探す | ||
| for (const item of clipboardItems) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
以下のような感じで、メソッドチェインにしてあげると分かりやすいかも?
(好みの問題な気もしますが)
const clipboardText = await Promise.any(
clipboardItems
.filter(item => item.types.includes("text/plain"))
.map(item => item.getType("text/plain").then(blob => blob.text()))
) || null;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terapotan
ありがとうございます!!なるほどたしかに!!
ただの好みなのですが、Promise周辺でfilter/map/reduce/forEach...を使うとポンコツなのでたまにやらかすのと、
catchが別途いる感じになったりなどあるので、forで回せればなと思っています!
…ustom_mimetype-2591
…thub.com/romot-co/voicevox into fix/copy_paste_with_custom_mimetype-2591
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where, when pasting notes into non‐sequencer contexts, the wrong clipboard data would be pasted. Key changes include adding new clipboard helper functions to copy and read notes with custom MIME types, refactoring the store actions to use these helper functions, and adjusting import paths for consistency.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/store/singing/clipboardHelper.ts | New helper functions to handle note copying and pasting with custom MIME types |
| src/store/singing/index.ts | Refactored clipboard actions to delegate to the new helper functions and updated import paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes the clipboard paste behavior by distinguishing between sequencer paste (which requires full note information) and general paste (which should include only lyrics).
- Updated import paths and refactored clipboard actions in the singing store
- Introduced helper functions for copying and reading notes from the clipboard
- Adjusted the paste command to use the new clipboard helper functions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/store/singing/index.ts | Refactored copy and paste actions to utilize the new clipboard helper |
| src/store/singing/clipboardHelper.ts | Added helper functions for handling clipboard operations with MIME types |
sigprogramming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューしました!(遅くなりすみません…!)
…thub.com/romot-co/voicevox into fix/copy_paste_with_custom_mimetype-2591
|
こういう場合のテストを考えてみた:
Claudeくんに聞いてみた |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/store/singing/clipboardHelper.ts:64
- [nitpick] Consider capturing the error in this catch block (e.g. catch (error)) and optionally logging it to provide more context for debugging failures in writing with the custom MIME type.
} catch {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/store/singing/clipboardHelper.ts:142
- [nitpick] The error message thrown in the catch block of validateNotesForClipboard does not differentiate between JSON parsing errors and schema validation errors. Consider separating these error cases to provide clearer troubleshooting information.
function validateNotesForClipboard(clipboardText: string): Omit<Note, "id">[] {
sigprogramming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
ChromeとFireFoxで動作確認しましたが、問題なさそうでした!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
|
@sevenc-nanashi さんすみません!! ちょっとENGINEの方でいっぱいレビューすることがあるので、こちらのレビューをお任せしても良いでしょうか 🙇 |
| // 選択されたトラックがない場合は何もしない | ||
| if (!selectedTrack) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これがundefinedになることは無さそう?
かなり難しいんですけど、とりあえずVOICEVOXとしてはelectron(chrome)でのe2eテストが行われていれば十分な気がしました! あるいはユニットテストにするのもありかもです! Lines 39 to 55 in 0c34bae
これでもしかしたらクリップボードのテストもできる・・・かも・・・? (でもブラウザの中からクリップボードにアクセスするのが難しくて、やっぱりe2eテストのが良いのかも!) |
|
ping @sevenc-nanashi |
src/store/singing/clipboardHelper.ts
Outdated
| type: "text/html", | ||
| }); | ||
| // 安全策としてのtext/plainの空文字 | ||
| const emptyTextBlob = new Blob([""], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
気持ち的に歌詞もコピーしてくれたほうが嬉しいかも?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
歌詞コピーにしました!
※ コピペは現実的にそんなに困らず優先度高くないと思うので、レビュー後回しでお願いします…!
| const clipboardItem = new ClipboardItem({ | ||
| [VOICEVOX_NOTES_MIME_TYPE]: notesBlob, | ||
| }); | ||
| await navigator.clipboard.write([clipboardItem]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(try VOICEVOX_NOTES_MIME_TYPE catch text/html) and text/plainみたいな感じに整理したほうが見やすいかも。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちら修正しました!
内容
ref: #2591
ソングにおいて以下の形で
ノートをコピーしてから歌詞入力などにペーストするとノートのJSONが貼り付けられてしまう問題を修正します。
ElectronエディタやChromeの場合
web application/vnd.voicevox.song-notesでノート構造をコピーしますブラウザ版でFirefoxやSafariなどの場合
text/htmlのdata属性にノート構造をコピーし、text/plainに空文字をセットしますtext/htmlのdata属性がペーストされますtext/htmlのタグが無視されるかtext/plainが優先されなにもペーストされません関連 Issue
ref #2591
close #2591
スクリーンショット・動画など
その他
クリップボードAPIの制限
https://developer.mozilla.org/ja/docs/Web/API/Clipboard_API#%E3%82%BB%E3%82%AD%E3%83%A5%E3%83%AA%E3%83%86%E3%82%A3%E3%81%AE%E8%80%83%E6%85%AE
Chrome 104以降でカスタムMIMEタイプが使えるAsync Clipboard API
https://developer.chrome.com/blog/web-custom-formats-for-the-async-clipboard-api?hl=ja